Skip to content

make improvments #587

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 28, 2023
Merged

make improvments #587

merged 3 commits into from
Feb 28, 2023

Conversation

honno
Copy link
Member

@honno honno commented Feb 1, 2023

  • make install now installs dependencies
  • build command renamed to spec
  • Introduced draft command aliasing sphinx-build spec/draft/ _site/draft/

README updated accordingly.

I'm not sure on general procedure with makefiles here: I assumed make install shouldn't be depended upon by the draft and spec commands.

@honno
Copy link
Member Author

honno commented Feb 1, 2023

FYI it seems CI doesn't like editable installs, but I think it's fair not to have those in CI jobs anyway, hence manually installing dependencies in the workflows as opposed to leaning on the new make.

@asmeurer
Copy link
Member

asmeurer commented Feb 1, 2023

I comment this on Slack (and also on the other PR), but let's please not require to install things to build the docs. It isn't necessary. My suggestion would be:

  • Move the common conf.py file to the spec/ directory. The other day I was looking for it and I couldn't find it because I was expecting it to be there.
  • Add sys.path.insert(0, '..') to the individual conf.py files so that they are able to import it (and sys.path.insert(0, '../../src') for the spec files). That way no installation or PYTHONPATH munging is necessary.

@asmeurer
Copy link
Member

asmeurer commented Feb 1, 2023

Not building against an installed version of the spec will also avoid potential confusion if someone has the release version of the spec installed and the docs build against that instead of the local dev version. There's no reason you'd ever want to build the docs against anything except for the local version.

@honno
Copy link
Member Author

honno commented Feb 2, 2023

Please don't do this. I would not expect make to go installing things in my current environment. It's possible to make the docs build without installing anything.

I had make do the make install part because of Ralf's comment making me concious of the longer invocation of the whole build process, but yes I see removing it makes sense with how make usually works. EDIT: done just that.

Move the common conf.py file to the spec/ directory. The other day I was looking for it and I couldn't find it because I was expecting it to be there.

That doesn't make sense considering spec/ isn't a Sphinx doc folder now, and shouldn't be given how valuable the separate docs are—from #505: "The main drive in having technically separate docs is to keep utilising Sphinx's autodoc, which is incredibly useful but ill-equipped of having multiple spec versions in the repo. The ability to monkey-patch sys.modules in the conf.py of every version-doc gives us consistency, basically just so we don't have to worry about modifying the .. currentmodule targets for every version."

Add sys.path.insert(0, '..') to the individual conf.py files so that they are able to import it (and sys.path.insert(0, '../../src') for the spec files). That way no installation or PYTHONPATH munging is necessary.

From #505 (comment): "I think actually we don't benefit much from magic here, as users still need to install the doc dependencies. Documenting in the README to use a single command pip install -e .[doc] doesn't seem too bad, even if just having to run pip install -r requirements.txt is more intuitive and what we're used to.

Having sys.path.append("../../array_api_stubs/YYYY_MM/"); import YYYY_MM; sys.modules["array_api"] = YYYY_MM for all conf.py files isn't much a problem and I see not uncommon, but it's still something in terms of maintenance burden and environment hacks. Promoting just a single way to work on both the array_api_stubs package and the spec documentation feels a bit nicer IMO as it's more true-to-form for a Sphinx doc and doesn't confuse what the stubs actually are.

Also I like the modern pyproject.toml process of the src/ folder which can contain multiple modules. Right now that's just _array_api_conf, which gets imported to each conf.py and saves us from path hacks per conf.py.

Concretely I'd like to utilise src/ for a small Python script which can be run to build redirect pages in the future (unfortunately sphinx-reredirects isn't workable for what we need). Even if we don't do that specifically, it's nice to have the flexibility (i.e. the local development instructions won't change if we introduce a module)."

Not building against an installed version of the spec will also avoid potential confusion if someone has the release version of the spec installed and the docs build against that instead of the local dev version. There's no reason you'd ever want to build the docs against anything except for the local version.

Yeah that's fair, although this is a potential problem when building most docs.

@asmeurer
Copy link
Member

From #505 (comment): "I think actually we don't benefit much from magic here, as users still need to install the doc dependencies. Documenting in the README to use a single command pip install -e .[doc] doesn't seem too bad, even if just having to run pip install -r requirements.txt is more intuitive and what we're used to.

There's a difference between installing a dependency which is just a standard Python package and installing some dev scripts.

Having sys.path.append("../../array_api_stubs/YYYY_MM/"); import YYYY_MM; sys.modules["array_api"] = YYYY_MM for all conf.py files isn't much a problem and I see not uncommon, but it's still something in terms of maintenance burden and environment hacks. Promoting just a single way to work on both the array_api_stubs package and the spec documentation feels a bit nicer IMO as it's more true-to-form for a Sphinx doc and doesn't confuse what the stubs actually are.

I don't see how it's a maintenance burden. I use this pattern all the time in various docs and I never need to do anything with it.

Also what is the sys.modules["array_api"] = ... bit for? Is that needed to trick autodoc? How is that different from installing the package?

Alternatively you can set PYTHONPATH in the Makefile. The difference is that would not support running sphinx-build directly, but people generally shouldn't do that.

Yeah that's fair, although this is a potential problem when building most docs.

That's why I like the sys.path.insert(0, ...) pattern in conf.py. It ensures that you always build the docs against the local version, regardless of what is installed.

@rgommers
Copy link
Member

That's why I like the sys.path.insert(0, ...) pattern in conf.py. It ensures that you always build the docs against the local version, regardless of what is installed.

This sounds like a useful pattern, and one that we've used in SciPy as well for a long time without any issues.

i.e. so you don't have to explicitly install things to build docs
@honno
Copy link
Member Author

honno commented Feb 23, 2023

Pushed 19ad87c which uses sys.path

  • sys.path.insert() the src/ folder per-conf.py.

    • Use str(Path()) to hopefully be a compatible approach on both unix and windows systems

    • Alternatively you can set PYTHONPATH in the Makefile. The difference is that would not support running sphinx-build directly, but people generally shouldn't do that.

      I think it's quite nice to support sphinx-build, as it: keeps with typical doc workflows, doesn't have make do magic beyond orchestrating more atomic commands, and doesn't spam make to have a command for building each individual version.

    Generally I don't think is worth the increase in complexity still prefer it to just doing sys.path.insert() once in make, but hey-ho.

  • Doc requirements now in doc-requirements.txt as opposed to the pyproject.toml (they're not necessary for any future package-building needs anywho)

  • Updated README accordingly


Also what is the sys.modules["array_api"] = ... bit for? Is that needed to trick autodoc? How is that different from installing the package?

Yep to trick autodoc so generated docs and urls are prefixed with array_api, rather than if you say installed the package and would have {array_api_stubs.}_2021.12, {array_api_stubs.}_draft etc. (didn't seem to be a nice way to magic these generated module paths into array_api otherwise at the time of #505).


Given this changes our GH/circleci workflows, I'll push this PR to honno#9 to see if at least the circleci preview works fine.

@honno honno requested review from asmeurer and rgommers February 25, 2023 14:09
@rgommers rgommers added the Maintenance Bug fix, typo fix, or general maintenance. label Feb 28, 2023
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM now and works smoothly. Thanks @honno! And thanks @asmeurer for the review.

"sphinx_copybutton",
"docutils<0.18",
"sphinx-math-dollar",
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is unfortunately needed because pip is still not able to install from this dependency list without also installing the project itself.

@rgommers rgommers merged commit d872032 into data-apis:main Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Bug fix, typo fix, or general maintenance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants